feat: implement large-room post restriction in message-gatekeeper#162
feat: implement large-room post restriction in message-gatekeeper#162
Conversation
📝 WalkthroughWalkthroughThis PR enforces a large-room post restriction in ChangesLarge-Room Post Restriction Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
message-gatekeeper/handler.go (1)
54-63: 💤 Low valueConstructor argument list is starting to grow — consider an options struct for future maintainability.
NewHandlernow takes six positional args (store, publish, reply, siteID, parentFetcher, largeRoomThreshold). Each subsequent gate added to the gatekeeper will further bloat this signature. AHandlerOptions(orConfig) struct passed by value/pointer would reduce call-site noise and prevent silent positional misordering of same-typed args (e.g., anotherintconfig in the future). Not a blocker for this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@message-gatekeeper/handler.go` around lines 54 - 63, The NewHandler constructor has too many positional arguments; create a new HandlerOptions (or Config) struct containing fields store, publish, reply, siteID, parentFetcher, and largeRoomThreshold, then change NewHandler to accept a single HandlerOptions (pointer or value) and use its fields to populate and return the Handler; update all call sites to construct and pass the options struct to avoid positional misordering and make future additions non-breaking.message-gatekeeper/store.go (1)
30-36: 💤 Low valueConsider broadening the user-facing message to reflect bypass policy.
The sentinel's
Messagesays "only owners can post in this room", but the bypass list now also includes admins and bots. End-users hitting this error in the future may be confused if/when the role-update RPC starts allowing admin assignments. A phrasing like "only owners and admins can post in this large room" would stay aligned with the comment on lines 30–32 without leaking implementation details (bot pattern).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@message-gatekeeper/store.go` around lines 30 - 36, The user-facing Message in errLargeRoomPostRestricted (a codedError with Code codeLargeRoomPostRestricted) is too specific ("only owners can post...") and should be broadened to match the bypass policy described in the comment; update the Message string in errLargeRoomPostRestricted to a neutral, accurate phrase such as "only owners and admins can post in this large room" (leave the variable name errLargeRoomPostRestricted and the Code field unchanged).message-gatekeeper/handler_test.go (1)
326-400: ⚡ Quick winFast-path success cases don't assert canonical publish actually happens.
The owner/admin/bot bypass cases set
wantErr: falseand capturepublished, but never assert the slice is non-empty (nocheckResult). A regression where the bypass path fell through to a code path that returns nil error without publishing would slip past these tests. Mirroring the existing happy-path checks (assert.Len(t, published, 1)) on the bypass cases would close that gap.♻️ Example tightening for the owner case (apply to admin/bot/thread-reply analogously)
setupPub: func() (publishFunc, *[]publishedMsg) { var published []publishedMsg return makePublishFunc(&published, nil), &published }, wantErr: false, + checkResult: func(t *testing.T, data []byte, published []publishedMsg) { + require.NotNil(t, data) + assert.Len(t, published, 1, "bypass path must still publish to MESSAGES_CANONICAL") + }, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@message-gatekeeper/handler_test.go` around lines 326 - 400, The three fast-path tests ("owner sends in big room — fast-path skips GetRoom", "admin sends in big room — fast-path skips GetRoom", "bot account in big room with member role — fast-path skips GetRoom") currently set wantErr: false and collect published via makePublishFunc but never assert that publishing actually occurred; update each case to assert the publish happened (e.g. call the existing check/assert helper used elsewhere or add assert.Len(t, *publishedPtr, 1)) after the handler runs so the test verifies published (the published slice returned by setupPub/makePublishFunc and the publishedMsg type) is non-empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@message-gatekeeper/main.go`:
- Line 33: The env tag for the LargeRoomThreshold config field uses a
non-prefixed name; change the struct tag on LargeRoomThreshold to use a
service-prefixed SCREAMING_SNAKE_CASE env var (e.g.,
MESSAGE_GATEKEEPER_LARGE_ROOM_THRESHOLD) instead of LARGE_ROOM_THRESHOLD, update
the envDefault if needed, and search for any other references to the old env
name to update them accordingly so the service follows the env-var naming
convention.
---
Nitpick comments:
In `@message-gatekeeper/handler_test.go`:
- Around line 326-400: The three fast-path tests ("owner sends in big room —
fast-path skips GetRoom", "admin sends in big room — fast-path skips GetRoom",
"bot account in big room with member role — fast-path skips GetRoom") currently
set wantErr: false and collect published via makePublishFunc but never assert
that publishing actually occurred; update each case to assert the publish
happened (e.g. call the existing check/assert helper used elsewhere or add
assert.Len(t, *publishedPtr, 1)) after the handler runs so the test verifies
published (the published slice returned by setupPub/makePublishFunc and the
publishedMsg type) is non-empty.
In `@message-gatekeeper/handler.go`:
- Around line 54-63: The NewHandler constructor has too many positional
arguments; create a new HandlerOptions (or Config) struct containing fields
store, publish, reply, siteID, parentFetcher, and largeRoomThreshold, then
change NewHandler to accept a single HandlerOptions (pointer or value) and use
its fields to populate and return the Handler; update all call sites to
construct and pass the options struct to avoid positional misordering and make
future additions non-breaking.
In `@message-gatekeeper/store.go`:
- Around line 30-36: The user-facing Message in errLargeRoomPostRestricted (a
codedError with Code codeLargeRoomPostRestricted) is too specific ("only owners
can post...") and should be broadened to match the bypass policy described in
the comment; update the Message string in errLargeRoomPostRestricted to a
neutral, accurate phrase such as "only owners and admins can post in this large
room" (leave the variable name errLargeRoomPostRestricted and the Code field
unchanged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4639664d-3a21-4909-a554-5a832e33ef9e
📒 Files selected for processing (14)
docs/superpowers/plans/2026-05-07-large-room-post-restriction.mddocs/superpowers/specs/2026-05-07-large-room-post-restriction-design.mdmessage-gatekeeper/handler.gomessage-gatekeeper/handler_test.gomessage-gatekeeper/helper.gomessage-gatekeeper/main.gomessage-gatekeeper/mock_store_test.gomessage-gatekeeper/store.gomessage-gatekeeper/store_mongo.gopkg/model/error.gopkg/model/model_test.gopkg/model/subscription.gopkg/natsutil/reply.gopkg/natsutil/reply_test.go
| MongoUsername string `env:"MONGO_USERNAME" envDefault:""` | ||
| MongoPassword string `env:"MONGO_PASSWORD" envDefault:""` | ||
| MaxWorkers int `env:"MAX_WORKERS" envDefault:"100"` | ||
| LargeRoomThreshold int `env:"LARGE_ROOM_THRESHOLD" envDefault:"500"` |
There was a problem hiding this comment.
Use a service-prefixed env var for the new threshold config.
Line 33 introduces a service-specific variable as LARGE_ROOM_THRESHOLD; this should be prefixed to avoid cross-service collisions and follow the config convention.
🔧 Suggested change
- LargeRoomThreshold int `env:"LARGE_ROOM_THRESHOLD" envDefault:"500"`
+ LargeRoomThreshold int `env:"MESSAGE_GATEKEEPER_LARGE_ROOM_THRESHOLD" envDefault:"500"`As per coding guidelines: “Environment variable names must be SCREAMING_SNAKE_CASE prefixed with service name for service-specific vars”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LargeRoomThreshold int `env:"LARGE_ROOM_THRESHOLD" envDefault:"500"` | |
| LargeRoomThreshold int `env:"MESSAGE_GATEKEEPER_LARGE_ROOM_THRESHOLD" envDefault:"500"` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@message-gatekeeper/main.go` at line 33, The env tag for the
LargeRoomThreshold config field uses a non-prefixed name; change the struct tag
on LargeRoomThreshold to use a service-prefixed SCREAMING_SNAKE_CASE env var
(e.g., MESSAGE_GATEKEEPER_LARGE_ROOM_THRESHOLD) instead of LARGE_ROOM_THRESHOLD,
update the envDefault if needed, and search for any other references to the old
env name to update them accordingly so the service follows the env-var naming
convention.
general-lex
force-pushed
the
claude/validate-message-sending-5HTd9
branch
from
1d40f93 to
01c7eaf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/superpowers/plans/2026-05-07-large-room-post-restriction.md (1)
1308-1312: 💤 Low valueProduction comment will be factually wrong — admin and bot bypass paths are omitted.
The comment block inserted into
handler.goat Step 7.6 states"only owners may send top-level messages"and"owner sends are exempt", but the actual bypass predicate also coversRoleAdminand bot accounts. This inaccuracy will mislead future maintainers.💡 Suggested fix
-// Large-room post restriction: in rooms with more than the configured -// threshold of members, only owners may send top-level messages. Thread -// replies are exempt regardless of room size; owner sends are exempt -// regardless of room size. Both bypasses skip the Room fetch entirely -// (approach B — owner fast-path). +// Large-room post restriction: in rooms with more than the configured +// threshold of members, only owners, admins, and bots may send top-level +// messages. Thread replies are exempt regardless of room size; bypass-eligible +// senders (owner/admin/bot) skip the Room fetch entirely (approach B — fast-path).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-05-07-large-room-post-restriction.md` around lines 1308 - 1312, The comment describing the "Large-room post restriction" in handler.go is inaccurate: the bypass predicate also allows RoleAdmin and bot accounts, not just owners. Update the comment block (the Step 7.6 large-room post restriction comment) to explicitly list RoleAdmin and bot accounts as additional bypasses alongside owners, retain that thread replies and owner sends are exempt, and note that these bypasses skip the Room fetch (owner fast-path) so the comment matches the actual predicate logic that checks owner, RoleAdmin, and bot accounts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-05-07-large-room-post-restriction.md`:
- Around line 1067-1097: The rejection test case "member sends in big room —
rejected with codedError" (and the two other reject cases) is missing a
checkResult that asserts no canonical publish occurred; update each reject case
to add a checkResult function that takes (t *testing.T, resp
*model.SendMessageResponse, publishedPtr *[]publishedMsg) and calls
assert.Empty(t, *publishedPtr) (and keep existing checkErr logic), ensuring you
use the same published variable returned by setupPub/makePublishFunc and the
publishedPtr parameter to verify no messages were published.
In `@docs/superpowers/specs/2026-05-07-large-room-post-restriction-design.md`:
- Around line 255-259: The user-facing error for errLargeRoomPostRestricted
currently reads "only owners can post in this room" but the bypass policy also
permits admins and bots; update the Message value on errLargeRoomPostRestricted
to a role-neutral phrase such as "posting is restricted in this room" (or
similar) and propagate the same wording to the implementation in store.go, the
spec's wire-format example, the assertion in the testing strategy table, and the
TestMarshalErrorWithCode test so the error text remains consistent across code,
docs, and tests.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-05-07-large-room-post-restriction.md`:
- Around line 1308-1312: The comment describing the "Large-room post
restriction" in handler.go is inaccurate: the bypass predicate also allows
RoleAdmin and bot accounts, not just owners. Update the comment block (the Step
7.6 large-room post restriction comment) to explicitly list RoleAdmin and bot
accounts as additional bypasses alongside owners, retain that thread replies and
owner sends are exempt, and note that these bypasses skip the Room fetch (owner
fast-path) so the comment matches the actual predicate logic that checks owner,
RoleAdmin, and bot accounts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73ca71c4-43a8-43c4-8d57-602f99c11e06
📒 Files selected for processing (14)
docs/superpowers/plans/2026-05-07-large-room-post-restriction.mddocs/superpowers/specs/2026-05-07-large-room-post-restriction-design.mdmessage-gatekeeper/handler.gomessage-gatekeeper/handler_test.gomessage-gatekeeper/helper.gomessage-gatekeeper/main.gomessage-gatekeeper/mock_store_test.gomessage-gatekeeper/store.gomessage-gatekeeper/store_mongo.gopkg/model/error.gopkg/model/model_test.gopkg/model/subscription.gopkg/natsutil/reply.gopkg/natsutil/reply_test.go
✅ Files skipped from review due to trivial changes (3)
- message-gatekeeper/helper.go
- pkg/model/subscription.go
- pkg/model/error.go
🚧 Files skipped from review as they are similar to previous changes (7)
- message-gatekeeper/mock_store_test.go
- message-gatekeeper/main.go
- message-gatekeeper/store_mongo.go
- message-gatekeeper/store.go
- pkg/natsutil/reply.go
- message-gatekeeper/handler.go
- message-gatekeeper/handler_test.go
| var errLargeRoomPostRestricted = &codedError{ | ||
| Code: "large_room_post_restricted", | ||
| Message: "only owners can post in this room", | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Misleading user-facing error message — admins and bots are also permitted senders.
The Message field says "only owners can post in this room", but the bypass predicate also allows RoleAdmin and bot accounts. While the admin clause is currently dormant, this message will surface to regular members and, once admin assignment is wired in room-service, could confuse an admin who receives a rejection for any other reason. Prefer a role-neutral phrasing that stays accurate as the bypass policy evolves.
💡 Suggested fix
-var errLargeRoomPostRestricted = &codedError{
- Code: "large_room_post_restricted",
- Message: "only owners can post in this room",
-}
+var errLargeRoomPostRestricted = &codedError{
+ Code: "large_room_post_restricted",
+ Message: "posting is restricted to owners and admins in this room",
+}The same change should be applied consistently in store.go (the actual implementation), the wire-format example in this spec (line 281), the assertion in the testing strategy table (line 352), and the TestMarshalErrorWithCode test in pkg/natsutil/reply_test.go (line 83).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var errLargeRoomPostRestricted = &codedError{ | |
| Code: "large_room_post_restricted", | |
| Message: "only owners can post in this room", | |
| } | |
| ``` | |
| var errLargeRoomPostRestricted = &codedError{ | |
| Code: "large_room_post_restricted", | |
| Message: "posting is restricted to owners and admins in this room", | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-05-07-large-room-post-restriction-design.md`
around lines 255 - 259, The user-facing error for errLargeRoomPostRestricted
currently reads "only owners can post in this room" but the bypass policy also
permits admins and bots; update the Message value on errLargeRoomPostRestricted
to a role-neutral phrase such as "posting is restricted in this room" (or
similar) and propagate the same wording to the implementation in store.go, the
spec's wire-format example, the assertion in the testing strategy table, and the
TestMarshalErrorWithCode test so the error text remains consistent across code,
docs, and tests.
mliu33
left a comment
mliu33
left a comment
There was a problem hiding this comment.
Great work, thanks! Just one minor suggestion~
| // mongo.ErrNoDocuments) is wrapped and returned — the handler treats every | ||
| // failure here as an infrastructure error, since reaching this call already | ||
| // implies a subscription for the room exists. | ||
| func (s *MongoStore) GetRoom(ctx context.Context, roomID string) (*model.Room, error) { |
There was a problem hiding this comment.
If the only usage is to get user count, maybe we can update the method name and add projection to return only "UserAcount" field for better performance ?
There was a problem hiding this comment.
Brief on the trade-off, since it's worth a thought:
For: every non-bypass send pays a Mongo round-trip to read Room.UserCount. With SetProjection({"userCount": 1}) we send/receive the _id plus a single int instead of the full ~13-field document. Per call it's a few hundred bytes; on a busy 600-person channel where most senders are regular members, it adds up. The narrower method name (GetRoomUserCount vs. GetRoom) also makes intent obvious — a future maintainer doesn't have to read the call site to know nothing else from Room is consumed.
Against: the other services (room-service, room-worker, broadcast-worker) all have a generic GetRoom that returns the full document, so this method is now slightly inconsistent. And if a future admission rule wants another Room field (Restricted, Type, etc.), we'd either expand the projection or split into a second method.
Net call: the consistency argument is weak when there's no concrete reuse — every service's Store is independent — and YAGNI on the future-rule case (cheaper to widen later than carry weight now). So:
Store.GetRoom→Store.GetRoomUserCount(ctx, roomID) (int, error)MongoStoredecodes into a single-field anonymous struct via the projection- All call sites, 16 mock expectations, regenerated mock
- Spec updated to match (
651fd2e)
Generated by Claude Code
The cap rule's only consumer of room data is userCount. Narrowing the
Store method to GetRoomUserCount(ctx, roomID) (int, error) and adding a
Mongo projection (`SetProjection({"userCount": 1})`) avoids pulling the
rest of the Room document over the wire on every non-bypass send.
Per-call savings are modest (~250B) but accumulate on the hot path. The
narrower name also signals intent — a future maintainer doesn't have to
read the call site to learn that nothing else from Room is consumed.
Addresses PR #162 review comment from @mliu33.
In rooms with more than 500 members, only owners may send top-level messages. Thread replies are exempt; edits and deletes are unaffected. Threshold configurable via env var. Single change point in message-gatekeeper plus a backward-compatible Code field on model.ErrorResponse.
…tant Bypass policy now allows owners, admins, and bots (account name matches \.bot$|^p_) to send in 500+ rooms. RoleAdmin is added to pkg/model as a constant only — role-update RPC support remains owned by another team. isBot is duplicated inline in message-gatekeeper rather than shared, to unblock this PR; promotion to pkg/botid is tracked as future work.
…rvice convention)
…mThreshold addition
Address findings from the final cross-task code review: - Add TestHandler_ProcessMessage case for GetRoom returning a wrapped mongo.ErrNoDocuments — a distinct sentinel from generic Mongo errors, must still classify as infraError per the spec table. - Add wantNoPublish field on the table struct and assertion in the runner so rule-rejection cases prove no canonical publish occurs when blocked.
…stant The wire code "large_room_post_restricted" was duplicated between the errLargeRoomPostRestricted sentinel and the slog "reason" field. Pull it into a single named constant so the error and the log line can never drift apart.
Address findings from the final code review: - RoleAdmin in pkg/model/subscription.go now carries an inline comment flagging that the constant is recognized by message-gatekeeper's bypass but not yet assignable via room-service's role-update RPC. - errLargeRoomPostRestricted's doc comment now correctly names all three bypass categories (owner, admin, bot) instead of just "non-owner".
- Broaden user-facing rejection message to "posting is restricted to owners and admins in this room" — admins are bypass-eligible too, so saying "only owners" is misleading. - Assert canonical publish actually fires on owner/admin/bot/thread-reply bypass paths so a regression that silently drops a bypassed message would fail the test.
The cap rule's only consumer of room data is userCount. Narrowing the
Store method to GetRoomUserCount(ctx, roomID) (int, error) and adding a
Mongo projection (`SetProjection({"userCount": 1})`) avoids pulling the
rest of the Room document over the wire on every non-bypass send.
Per-call savings are modest (~250B) but accumulate on the hot path. The
narrower name also signals intent — a future maintainer doesn't have to
read the call site to learn that nothing else from Room is consumed.
Addresses PR #162 review comment from @mliu33.
PR #161 (1807b75) introduced two undefined-symbol build breaks: - mongorepo/room.go used Collection / NewCollection / WithProjection without importing pkg/mongoutil (the sibling subscription.go and threadroom.go do so correctly with a "mongoutil." prefix). - service/service.go's compile-time interface assertion referenced mongorepo.RoomRepo without importing the mongorepo package. Both go build and golangci-lint fail on origin/main HEAD because of this. Without the fix every PR that merges with main inherits the break — including this one. Adding the imports + qualifiers locally unblocks our PR's CI and makes the merge clean.
general-lex
force-pushed
the
claude/validate-message-sending-5HTd9
branch
from
26ea5a9 to
e34b362
Compare
…shipped
- Predicate sample uses GetRoomUserCount(int) and the
codeLargeRoomPostRestricted constant.
- Store interface section reflects the renamed method and the
SetProjection({"userCount":1}) optimization.
- Error sentinel and wire-format examples show the broadened "posting
is restricted to owners and admins in this room" message.
- Logging section renames the constant reference and the GetRoom
failure note.
- Testing strategy table renames the mock-expectation column,
documents the wantNoPublish field, and the bypass-case
assert.Len(t, published, 1) assertion.
Plan doc is intentionally left as-is — it's a one-shot implementation
checklist whose value as a historical record lives in the git log.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
message-gatekeeper/main.go (1)
33-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a service-prefixed env var for threshold config.
Line 33 still uses
LARGE_ROOM_THRESHOLD; this is service-specific and should be prefixed (for example:MESSAGE_GATEKEEPER_LARGE_ROOM_THRESHOLD) to avoid cross-service collisions.Suggested patch
- LargeRoomThreshold int `env:"LARGE_ROOM_THRESHOLD" envDefault:"500"` + LargeRoomThreshold int `env:"MESSAGE_GATEKEEPER_LARGE_ROOM_THRESHOLD" envDefault:"500"`As per coding guidelines: “Environment variable names must be
SCREAMING_SNAKE_CASEprefixed with service name for service-specific vars”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@message-gatekeeper/main.go` at line 33, The env tag for the LargeRoomThreshold config field uses a generic name `LARGE_ROOM_THRESHOLD`; change it to a service-prefixed env var (e.g. `MESSAGE_GATEKEEPER_LARGE_ROOM_THRESHOLD`) in the struct tag for LargeRoomThreshold so the env loader reads the prefixed variable; update any config loading, documentation, and tests that reference the old `LARGE_ROOM_THRESHOLD` name to the new `MESSAGE_GATEKEEPER_LARGE_ROOM_THRESHOLD` name to avoid cross-service collisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-05-07-large-room-post-restriction.md`:
- Around line 183-200: Update the plan text to match the shipped API: replace
all references to GetRoom with GetRoomUserCount and show the correct signature
GetRoomUserCount(ctx context.Context, roomID string) (int, error); update the
example Store interface block and any checklist items/step descriptions that
mention GetRoom (including "Add `GetRoom` to the `Store` interface") so they
reference Store and GetRoomUserCount instead, and adjust the note about
regenerating mocks to reflect the actual method name so contributors implement
against the current API.
In `@docs/superpowers/specs/2026-05-07-large-room-post-restriction-design.md`:
- Around line 171-173: Update the error-classification text to reference the
current method name: replace mentions of GetRoom with GetRoomUserCount so it
accurately describes where `*infraError` (NACK + JetStream redelivery) applies;
keep the rest of the sentence intact (i.e., plain validation error
`errLargeRoomPostRestricted` (ACK + reply) for non-owner-in-big-room) to mirror
how `GetSubscription` errors are classified.
- Around line 37-47: The fenced code block that shows the pipeline diagram
starting with "Subject parse → SiteID match → unmarshal → ID/content validation"
is missing a language tag and triggers MD040; add an appropriate language
identifier (for example "text") to the opening fence (i.e., change ``` to
```text) so the block is lintable while keeping the diagram content unchanged.
---
Duplicate comments:
In `@message-gatekeeper/main.go`:
- Line 33: The env tag for the LargeRoomThreshold config field uses a generic
name `LARGE_ROOM_THRESHOLD`; change it to a service-prefixed env var (e.g.
`MESSAGE_GATEKEEPER_LARGE_ROOM_THRESHOLD`) in the struct tag for
LargeRoomThreshold so the env loader reads the prefixed variable; update any
config loading, documentation, and tests that reference the old
`LARGE_ROOM_THRESHOLD` name to the new `MESSAGE_GATEKEEPER_LARGE_ROOM_THRESHOLD`
name to avoid cross-service collisions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4dbf9df0-b7f8-4a0a-a621-caa2cff43f88
📒 Files selected for processing (16)
docs/superpowers/plans/2026-05-07-large-room-post-restriction.mddocs/superpowers/specs/2026-05-07-large-room-post-restriction-design.mdhistory-service/internal/mongorepo/room.gohistory-service/internal/service/service.gomessage-gatekeeper/handler.gomessage-gatekeeper/handler_test.gomessage-gatekeeper/helper.gomessage-gatekeeper/main.gomessage-gatekeeper/mock_store_test.gomessage-gatekeeper/store.gomessage-gatekeeper/store_mongo.gopkg/model/error.gopkg/model/model_test.gopkg/model/subscription.gopkg/natsutil/reply.gopkg/natsutil/reply_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/model/error.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/model/model_test.go
- message-gatekeeper/mock_store_test.go
- message-gatekeeper/helper.go
- message-gatekeeper/handler.go
- message-gatekeeper/store.go
- message-gatekeeper/handler_test.go
- pkg/natsutil/reply.go
| ## Task 3: Add `GetRoom` to gatekeeper `Store` + `MongoStore` impl + regen mocks | ||
|
|
||
| **Files:** | ||
| - Modify: `message-gatekeeper/store.go` | ||
| - Modify: `message-gatekeeper/store_mongo.go` | ||
| - Regenerate: `message-gatekeeper/mock_store_test.go` | ||
|
|
||
| (No new test for `GetRoom` itself — `MongoStore` implementations in this repo are exercised through handler tests with mocked stores, and the spec explicitly omits integration tests for this rule.) | ||
|
|
||
| - [ ] **Step 3.1: Add `GetRoom` to the `Store` interface** | ||
|
|
||
| In `message-gatekeeper/store.go`, replace the existing `Store` interface block with: | ||
|
|
||
| ```go | ||
| type Store interface { | ||
| GetSubscription(ctx context.Context, account, roomID string) (*model.Subscription, error) | ||
| GetRoom(ctx context.Context, roomID string) (*model.Room, error) | ||
| } |
There was a problem hiding this comment.
Plan steps still reference the old GetRoom API.
This section instructs implementing GetRoom(ctx, roomID) (*model.Room, error), but the finalized change uses GetRoomUserCount(ctx, roomID) (int, error). Please align the plan text to the shipped API so future contributors don’t implement against stale instructions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-05-07-large-room-post-restriction.md` around
lines 183 - 200, Update the plan text to match the shipped API: replace all
references to GetRoom with GetRoomUserCount and show the correct signature
GetRoomUserCount(ctx context.Context, roomID string) (int, error); update the
example Store interface block and any checklist items/step descriptions that
mention GetRoom (including "Add `GetRoom` to the `Store` interface") so they
reference Store and GetRoomUserCount instead, and adjust the note about
regenerating mocks to reflect the actual method name so contributors implement
against the current API.
| ``` | ||
| Subject parse → SiteID match → unmarshal → ID/content validation | ||
| ↓ | ||
| GetSubscription (existing) | ||
| ↓ | ||
| [NEW] LARGE-ROOM CHECK | ||
| ↓ | ||
| resolveQuoteSnapshot (existing) | ||
| ↓ | ||
| Build Message + publish to MESSAGES_CANONICAL | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to fenced code block.
The code fence starting at Line 37 is missing a language identifier, which triggers markdown lint (MD040).
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-05-07-large-room-post-restriction-design.md`
around lines 37 - 47, The fenced code block that shows the pipeline diagram
starting with "Subject parse → SiteID match → unmarshal → ID/content validation"
is missing a language tag and triggers MD040; add an appropriate language
identifier (for example "text") to the opening fence (i.e., change ``` to
```text) so the block is lintable while keeping the diagram content unchanged.
| `*infraError` (NACK + JetStream redelivery) for `GetRoom` failures. Plain | ||
| validation error (`errLargeRoomPostRestricted`, ACK + reply) for | ||
| non-owner-in-big-room. Mirrors how `GetSubscription` errors are classified |
There was a problem hiding this comment.
Use the updated method name in error-classification text.
Line 171 references GetRoom failures, but the implementation now enforces this via GetRoomUserCount. Updating the term here avoids confusion during debugging and test maintenance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-05-07-large-room-post-restriction-design.md`
around lines 171 - 173, Update the error-classification text to reference the
current method name: replace mentions of GetRoom with GetRoomUserCount so it
accurately describes where `*infraError` (NACK + JetStream redelivery) applies;
keep the rest of the sentence intact (i.e., plain validation error
`errLargeRoomPostRestricted` (ACK + reply) for non-owner-in-big-room) to mirror
how `GetSubscription` errors are classified.
3 participants
Summary
Implements a large-room post restriction that prevents non-owner/non-admin members from sending top-level messages in rooms exceeding a configurable member threshold (default 500). Thread replies and bot accounts are exempt. The check is added to
message-gatekeeperas an admission gate before publishing toMESSAGES_CANONICAL.Key Changes
Model & Shared Utilities
Codefield tomodel.ErrorResponsewithomitemptytag to preserve wire format for existing callersRoleAdminconstant topkg/model/subscription.gofor bypass eligibility (assignment wiring owned by separate team)MarshalErrorWithCodehelper topkg/natsutilfor typed error repliesMessage Gatekeeper
GetRoommethod toStoreinterface and implement inMongoStoreto fetch room member countsLargeRoomThresholdconfiguration (env:LARGE_ROOM_THRESHOLD, default: 500)canBypassLargeRoomCappredicate that exempts owners, admins, and bot accounts (detected via regex pattern\.bot$|^p_)codedErrortype anderrLargeRoomPostRestrictedsentinel for validation errors with machine-readable codesmarshalErrorReplydispatch helper to route errors through appropriate marshaling (coded vs. uncoded)processMessageafter subscription lookup but before quote resolution, skippingGetRoomfetch entirely for bypass-eligible senders (zero added cost for owners, admins, bots, and thread replies)isBothelper (duplicatesroom-service/helper.goconvention; future cleanup to sharedpkg/botidacknowledged)Tests
canBypassLargeRoomCap,isBot, andmarshalErrorReplydispatch logicmake generate SERVICE=message-gatekeeperImplementation Details
GetRoomlookup, so owners, admins, bots, and thread replies pay zero added Mongo costGetRoomfailures trigger infrastructure error (NACK + redelivery); large-room rejection is a validation error (ACK + coded reply)omitemptyonCodefield ensures existing error reply consumers see identical wire formatuserCount > threshold), so rooms at exactly the threshold are allowedhttps://claude.ai/code/session_017HzARgaSEatexstYkuvnnw
Summary by CodeRabbit